fix(ScrollArea): reset scroll only on direct viewport child swaps (#1349)#2045
fix(ScrollArea): reset scroll only on direct viewport child swaps (#1349)#2045kotAPI wants to merge 2 commits into
Conversation
|
📝 WalkthroughWalkthrough
ChangesScrollArea viewport child swap detection
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
CoverageThis report compares the PR with the base branch. "Δ" shows how the PR affects each metric.
Coverage improved or stayed the same. Great job! Run |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/ui/ScrollArea/fragments/ScrollAreaRoot.tsx`:
- Around line 69-74: The direct reset predicate in ScrollAreaRoot is too broad
because it treats any childList change on the viewport as a swap. Update the
mutation handling around directChildSwap to inspect the full mutations batch and
only trigger a reset when the viewport has both addedNodes and removedNodes,
indicating an actual child replacement. Keep the logic localized to the viewport
mutation check in ScrollAreaRoot.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 963057b3-7f69-468d-accb-8f4228036bdd
📒 Files selected for processing (2)
src/components/ui/ScrollArea/fragments/ScrollAreaRoot.tsxsrc/components/ui/ScrollArea/tests/ScrollArea.test.tsx
| const directChildSwap = mutations.some( | ||
| (mutation) => | ||
| mutation.type === 'childList' | ||
| && mutation.target === viewport | ||
| && (mutation.addedNodes.length > 0 || mutation.removedNodes.length > 0) | ||
| ); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Narrow the reset predicate to actual child replacements.
directChildSwap currently fires for any direct child add/remove, so appending or removing an item directly under the viewport will reset the user to the top. Track added and removed nodes across the batch and reset only when both occur for the viewport.
🐛 Proposed predicate tightening
- const directChildSwap = mutations.some(
- (mutation) =>
- mutation.type === 'childList'
- && mutation.target === viewport
- && (mutation.addedNodes.length > 0 || mutation.removedNodes.length > 0)
- );
+ const viewportChildMutations = mutations.filter(
+ mutation => mutation.type === 'childList' && mutation.target === viewport
+ );
+ const directChildSwap =
+ viewportChildMutations.some(mutation => mutation.addedNodes.length > 0)
+ && viewportChildMutations.some(mutation => mutation.removedNodes.length > 0);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const directChildSwap = mutations.some( | |
| (mutation) => | |
| mutation.type === 'childList' | |
| && mutation.target === viewport | |
| && (mutation.addedNodes.length > 0 || mutation.removedNodes.length > 0) | |
| ); | |
| const viewportChildMutations = mutations.filter( | |
| mutation => mutation.type === 'childList' && mutation.target === viewport | |
| ); | |
| const directChildSwap = | |
| viewportChildMutations.some(mutation => mutation.addedNodes.length > 0) | |
| && viewportChildMutations.some(mutation => mutation.removedNodes.length > 0); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/ui/ScrollArea/fragments/ScrollAreaRoot.tsx` around lines 69 -
74, The direct reset predicate in ScrollAreaRoot is too broad because it treats
any childList change on the viewport as a swap. Update the mutation handling
around directChildSwap to inspect the full mutations batch and only trigger a
reset when the viewport has both addedNodes and removedNodes, indicating an
actual child replacement. Keep the logic localized to the viewport mutation
check in ScrollAreaRoot.
| act(() => { | ||
| const nested = document.createElement('span'); | ||
| nested.textContent = 'nested'; | ||
| screen.getByTestId('panel-a').appendChild(nested); | ||
| }); | ||
|
|
||
| expect(viewport.scrollTop).toBe(120); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Flush the nested mutation before the negative assertion.
MutationObserver callbacks are async, so Line 198 can pass before a broken nested-mutation reset runs. Await a microtask after appending the nested node, then assert the scroll position.
💚 Proposed test hardening
act(() => {
const nested = document.createElement('span');
nested.textContent = 'nested';
screen.getByTestId('panel-a').appendChild(nested);
});
+ await act(async() => {
+ await Promise.resolve();
+ });
+
expect(viewport.scrollTop).toBe(120);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| act(() => { | |
| const nested = document.createElement('span'); | |
| nested.textContent = 'nested'; | |
| screen.getByTestId('panel-a').appendChild(nested); | |
| }); | |
| expect(viewport.scrollTop).toBe(120); | |
| act(() => { | |
| const nested = document.createElement('span'); | |
| nested.textContent = 'nested'; | |
| screen.getByTestId('panel-a').appendChild(nested); | |
| }); | |
| await act(async() => { | |
| await Promise.resolve(); | |
| }); | |
| expect(viewport.scrollTop).toBe(120); |
Code reviewReviewed and pushed follow-up fixes addressing handler composition, test patterns ( CI was green before fixes; please re-run checks on latest commit. |
1 participant
Scopes MutationObserver to viewport direct children (subtree: false) and resets scroll on panel swaps.\n\nCloses #1349
Summary by CodeRabbit